Skip to content

Conversation

@dmsnell
Copy link
Member

@dmsnell dmsnell commented Feb 21, 2023

When the HTML API was introduced a number of fields were switched from private visibility to protected so that Gutenberg and other systems could more easily enhance the behaviors through subclassing. The $this->html property was overlooked but important for systems using the Tag Processor to stich HTML, specifically performing operations on innerHTML and innerText.

This patch updates that visibility to what it should have been during the introducing of the API in 39bfc25.

Trac ticket: https://core.trac.wordpress.org/ticket/57575.

cc: @adamziel @ockham @azaozz

When the HTML API was introduced a number of fields were switched from `private` visibility
to `protected` so that Gutenberg and other systems could more easily enhance the behaviors
through subclassing. The `$this->html` property was overlooked but important for systems
using the Tag Processor to stich HTML, specifically performing operations on `innerHTML`
and `innerText`.

This patch updates that visibility to what it should have been during the introducing of
the API in 39bfc25.
@dmsnell dmsnell force-pushed the html-api/protected-html-field branch from 62c3014 to db398cb Compare February 21, 2023 23:41
@dmsnell
Copy link
Member Author

dmsnell commented Feb 21, 2023

@azaozz I included you here because I'd like to try and get this into 6.2 still. Should I create a Trac ticket for it?

@gziolo
Copy link
Member

gziolo commented Feb 22, 2023

We can reuse https://core.trac.wordpress.org/ticket/57575. See a similar follow-up in https://core.trac.wordpress.org/changeset/55206. I'll take care of it.

@gziolo
Copy link
Member

gziolo commented Feb 22, 2023

Committed with https://core.trac.wordpress.org/changeset/55402.

@gziolo gziolo closed this Feb 22, 2023
@adamziel
Copy link
Contributor

adamziel commented Feb 22, 2023

Just noting I'm running into access issues now in my HTML parsing PR and it would be handy to have a few more properties marked as protected, e.g. $bookmarks. It doesn't matter for 6.2, though, as we still don't know the final shape of the new API or whether it will come to fruition at all.

@dmsnell
Copy link
Member Author

dmsnell commented Feb 22, 2023

@adamziel $bookmarks is already protected in Core.
are you using a pre-merge version of the code?

@adamziel
Copy link
Contributor

Oh! Hm, perhaps I am. Thanks!

@dmsnell dmsnell deleted the html-api/protected-html-field branch February 22, 2023 22:24
@azaozz
Copy link
Contributor

azaozz commented Feb 28, 2023

@azaozz I included you here because I'd like to try and get this into 6.2 still. Should I create a Trac ticket for it?

Uh, sorry for the late response, seeing this just now :(

Yes, as the HTML API is already in core, any bugfixes or "code cleanup" can be added during beta. Bugfixes can also be added during RC but it is usually reserved for "bigger" bugs and edge cases, for example if there's a fatal error in some circumstances, etc.

For small changes like this one there's usually no need for a new ticket. New trac tickets are good for larger bugfixes, and for bugs that are fixed during RC.

Documentation (docblock) and tests can be added at any time and usually it is a good idea to have new tickets for them especially if the changes are more substantial.

ockham added a commit to WordPress/gutenberg that referenced this pull request Mar 2, 2023
In the 6.2 compat layer, make `WP_HTML_Tag_Processor`'s `$html` member `protected` to support subclassing.

This is for parity with Code in WordPress 6.2, see WordPress/wordpress-develop#4112 and [r55402](https://core.trac.wordpress.org/changeset/55402).
ockham added a commit to WordPress/gutenberg that referenced this pull request Mar 2, 2023
In the 6.2 compat layer, make `WP_HTML_Tag_Processor`'s `$html` member `protected` to support subclassing.

This is for parity with Code in WordPress 6.2, see WordPress/wordpress-develop#4112 and [r55402](https://core.trac.wordpress.org/changeset/55402).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants